-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix position of hover tooltip + Layer fixes #2159
Fix position of hover tooltip + Layer fixes #2159
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @cphelefu and @jolevesq)
packages/geoview-core/src/core/components/hover-tooltip/hover-tooltip.tsx
line 37 at r1 (raw file):
const hoverFeatureInfo = useMapHoverFeatureInfo(); const pointerPosition = useMapPointerPosition(); const mapElem = document.getElementById(`mapTargetElement-${mapId}`) as HTMLElement;
We need to use store hook useAppGeoviewHTMLElement()
instead of document.getElementId
packages/geoview-core/src/core/components/layers/left-panel/single-layer.tsx
line 40 at r1 (raw file):
import { ArrowDownwardIcon, ArrowUpIcon, TableViewIcon } from '@/ui/icons'; import { Divider } from '@/ui/divider/divider'; import { Typography } from '@/ui/typography/typography';
this can be written as import { Typography } from '@/ui';
packages/geoview-core/src/core/components/notifications/notifications.tsx
line 65 at r1 (raw file):
const { removeNotification } = useAppStoreActions(); useEffect(() => {
can u please add log like logger.logTraceUseEffect('Export Modal - mount');
packages/geoview-core/src/core/components/notifications/notifications.tsx
line 74 at r1 (raw file):
useEffect(() => { if (hasNewNotification) {
can u please add log like logger.logTraceUseEffect('Export Modal - mount');
packages/geoview-core/src/core/components/notifications/notifications.tsx
line 156 at r1 (raw file):
> {!hasNewNotification && ( <div>
Can u please replace div with Box
Previously, kaminderpal (Kamy) wrote…
This returns a different element. I basically used what is used across other components for getting map element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 5 unresolved discussions (waiting on @jolevesq and @kaminderpal)
packages/geoview-core/src/core/components/layers/left-panel/single-layer.tsx
line 40 at r1 (raw file):
Previously, kaminderpal (Kamy) wrote…
this can be written as
import { Typography } from '@/ui';
Done.
packages/geoview-core/src/core/components/notifications/notifications.tsx
line 65 at r1 (raw file):
Previously, kaminderpal (Kamy) wrote…
can u please add log like
logger.logTraceUseEffect('Export Modal - mount');
Done.
packages/geoview-core/src/core/components/notifications/notifications.tsx
line 74 at r1 (raw file):
Previously, kaminderpal (Kamy) wrote…
can u please add log like
logger.logTraceUseEffect('Export Modal - mount');
Done.
packages/geoview-core/src/core/components/notifications/notifications.tsx
line 156 at r1 (raw file):
Previously, kaminderpal (Kamy) wrote…
Can u please replace div with
Box
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @cphelefu and @jolevesq)
packages/geoview-core/src/core/components/hover-tooltip/hover-tooltip.tsx
line 37 at r1 (raw file):
Previously, cphelefu (Christopher Phelefu) wrote…
This returns a different element. I basically used what is used across other components for getting map element
u can do like this useAppGeoviewHTMLElement().querySelector('\[id^="
mapTargetElement-${mapId}`-"]')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @jolevesq and @kaminderpal)
packages/geoview-core/src/core/components/hover-tooltip/hover-tooltip.tsx
line 37 at r1 (raw file):
Previously, kaminderpal (Kamy) wrote…
u can do like this
useAppGeoviewHTMLElement().querySelector('\[id^="
mapTargetElement-${mapId}`-"]')
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @jolevesq)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 4 files at r1, 2 of 3 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @cphelefu)
7559348
into
Canadian-Geospatial-Platform:develop
Description
This PR fixes the issues with the hover tooltip. Add animation to notification bell.
Fixes #2156
Fixes #2158
Fixes #2160
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
https://cphelefu.github.io/geoview/
Checklist:
I have made corresponding changes to the documentationI have added tests that prove my fix is effective or that my feature worksNew and existing unit tests pass locally with my changesThis change is